Switched from ip to tz for geolocation #13636
Conversation
|
Demo starting at https://ubuntu-com-13636.demos.haus |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13636 +/- ##
==========================================
- Coverage 74.46% 74.43% -0.03%
==========================================
Files 107 107
Lines 2847 2848 +1
Branches 948 948
==========================================
Hits 2120 2120
- Misses 703 704 +1
Partials 24 24
|
samhotep
force-pushed
the
use-tz-for-geolocation
branch
from
3201797
to
987bcc0
Compare
There was a problem hiding this comment.
This works for your example with telephone numbers, but because /user-country.json needs the query arg, this is breaking takeovers geolocation
ubuntu.com/templates/base_index.html
Line 1417 in 8ac389f
You may need to do the Intl.DateTimeFormat().resolvedOptions().timeZone; for takeovers too
webapp/app.py
Outdated
| @@ -628,7 +628,7 @@ def takeovers_index(): | |||
| core_services_guide.init_app(app) | |||
|
|
|||
|
|
|||
| app.add_url_rule("/user-country.json", view_func=get_user_country_by_ip) | |||
| app.add_url_rule("/user-country.json", view_func=get_user_country_by_tz) | |||
There was a problem hiding this comment.
If you are replacing this completely, then you might as well delete get_user_country_by_ip function
There was a problem hiding this comment.
Ok, but will do separately when updating the takeovers as well
| @@ -1071,6 +1072,27 @@ def thank_you(): | |||
| ) | |||
|
|
|||
|
|
|||
| def get_user_country_by_tz(): | |||
There was a problem hiding this comment.
Can you also write a comment here for this function explaining how it works? I guess the key of this system is to use Intl.DateTimeFormat().resolvedOptions().timeZone;
I'd also write a Python test. This would not only test that nothing breaks in subsequent changes, but also as an example of how it works.
|
Also can you remove his |
carkod
added
Review: QA +1
Review: Code +1
and removed
Review: Code needed
Review: QA -1
labels
Done
QA
Issue / Card
Fixes WD-1173
Screenshots
Help
QA steps - Commit guidelines
Follow up
Should we also auto-fill the country input?